Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tolerations not parsing its options correctly, update docs regarding quoted driver options #1053

Merged
merged 1 commit into from
Apr 22, 2022

Conversation

szeber
Copy link

@szeber szeber commented Apr 8, 2022

Followup for #1045. Updates the docs with correct examples on how to quote multiple node selectors or tolerations and removes unquoting from the tolerations, since the CSV parser that parses the driver-opts already handles unquoting.

@szeber
Copy link
Author

szeber commented Apr 8, 2022

@tonistiigi @AkihiroSuda this should fix the issue raised by @ktaborski in #1045

@AkihiroSuda
Copy link
Collaborator

Can we have a unit test?

@ktaborski
Copy link

@AkihiroSuda @szeber any updates regarding this?

@ddelange
Copy link

ddelange commented Apr 11, 2022

edit: confirming this branch fixes my issues with current master

`invalid syntax`

FYI, when I quote as suggested here (running current master), I get an obscure invalid syntax error. Not sure if related as it's the first time I'm trying out the kubernetes driver (and we only have nodes with taints)

# wrong quotes
$ /usr/bin/docker buildx create --name builder-aead224e-ed8e-49fc-ad7c-b7232da1b586 --driver kubernetes --driver-opt tolerations="key=arch,value=arm64,operator=Equal,effect=NoSchedule" --driver-opt namespace=docker-buildx --driver-opt replicas=1 --driver-opt requests.cpu=1m --driver-opt limits.memory=2Gi --driver-opt nodeselector=kubernetes.io/arch=arm64 --driver-opt rootless=true --driver-opt qemu.install=true --buildkitd-flags --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host --use
  error: parse error on line 1, column 13: bare " in non-quoted-field

...

# wrong quotes
$ /usr/bin/docker buildx create --name builder-84bc1c8c-e0a5-48d8-9269-409422fa4490 --driver kubernetes --driver-opt 'tolerations="key=arch,value=arm64,operator=Equal,effect=NoSchedule"' --driver-opt namespace=docker-buildx --driver-opt replicas=1 --driver-opt requests.cpu=1m --driver-opt limits.memory=2Gi --driver-opt nodeselector=kubernetes.io/arch=arm64 --driver-opt rootless=true --driver-opt qemu.install=true --buildkitd-flags --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host --use
  error: parse error on line 1, column 14: bare " in non-quoted-field

...

# wrong quotes
$ /usr/bin/docker buildx create --name builder-123f035e-7fcc-4c81-b046-8ffadcb12833 --driver kubernetes --driver-opt "tolerations=\"key=arch,value=arm64,operator=Equal,effect=NoSchedule\"" --driver-opt namespace=docker-buildx --driver-opt replicas=1 --driver-opt requests.cpu=1m --driver-opt limits.memory=2Gi --driver-opt nodeselector=kubernetes.io/arch=arm64 --driver-opt rootless=true --driver-opt qemu.install=true --buildkitd-flags --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host --use
  error: parse error on line 1, column 15: extraneous or missing " in quoted-field

...

# quotes as suggested here, getting invalid syntax error
$ /usr/bin/docker buildx create --name builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb --driver kubernetes --driver-opt "tolerations=key=arch,value=arm64,operator=Equal,effect=NoSchedule" --driver-opt namespace=docker-buildx --driver-opt replicas=1 --driver-opt requests.cpu=1m --driver-opt limits.memory=2Gi --driver-opt nodeselector=kubernetes.io/arch=arm64 --driver-opt rootless=true --driver-opt qemu.install=true --buildkitd-flags --allow-insecure-entitlement security.insecure --allow-insecure-entitlement network.host --use
  builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb
$ /usr/bin/docker buildx inspect --bootstrap --builder builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb
  Name:   builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb
  Driver: kubernetes
  
  Nodes:
  Name:            builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb0
  Endpoint:        kubernetes:///builder-12f140fa-5dd7-45f7-87b2-59ceacbfe3fb?deployment=&kubeconfig=
  Driver Options:  limits.memory=2Gi
  ,                namespace=docker-buildx
  ,                nodeselector=kubernetes.io/arch=arm64
  ,                qemu.install=true
  ,                replicas=1
  ,                requests.cpu=1m
  ,                rootless=true
  ,                tolerations=key=arch,value=arm64,operator=Equal,effect=NoSchedule
  Error:           invalid syntax
$ /usr/bin/docker buildx bake --file docker-compose.yml --set *.platform=linux/amd64 --set *.platform=linux/arm64 --metadata-file /tmp/docker-build-push-orx3PF/metadata-file
error: no valid drivers found: invalid syntax

@szeber
Copy link
Author

szeber commented Apr 11, 2022

hey, i'll try to get a test in this today

@szeber
Copy link
Author

szeber commented Apr 12, 2022

@tonistiigi @AkihiroSuda Added some tests, it required a bit of refactoring of the factory to make this properly testable

@AkihiroSuda
Copy link
Collaborator

Please squash commits

@szeber szeber force-pushed the kubernetes-tolerations-fix branch from b9111ad to 4d88bde Compare April 12, 2022 01:44
@szeber
Copy link
Author

szeber commented Apr 12, 2022

@AkihiroSuda squashed

@ddelange
Copy link

@szeber could you fix merge conflicts?

@szeber szeber force-pushed the kubernetes-tolerations-fix branch from 4d88bde to 4a22656 Compare April 22, 2022 00:21
@szeber
Copy link
Author

szeber commented Apr 22, 2022

@ddelange fixed, it was fixing the same bug as i was :D

Correction. One of the same bugs as I was, this fixes other things too :)

@tonistiigi tonistiigi merged commit a6a1a36 into docker:master Apr 22, 2022
@crazy-max crazy-max added this to the v0.9.0 milestone Jun 5, 2022
@ddelange
Copy link

@crazy-max is there an expected release date planned?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants